4987 annual survey export#5540
Conversation
| year_start = [range_params[:year_start].to_i, current_organization.earliest_reporting_year].max | ||
| year_end = [range_params[:year_end].to_i, Time.current.year].min | ||
|
|
||
| year_start, year_end = [year_start, year_end].minmax |
There was a problem hiding this comment.
We probably want to fail loudly if end < start, not silently swap them.
|
|
||
| private | ||
|
|
||
| def get_range_report(year_start, year_end) |
There was a problem hiding this comment.
Prefer methods that don't start with get. In this case, probably also better to move this method to the service class.
| not_found! unless range_params[:year_start] =~ year_regex && range_params[:year_end] =~ year_regex | ||
| end | ||
|
|
||
| def year_regex |
There was a problem hiding this comment.
This should be a constant, not a method.
There was a problem hiding this comment.
Wait, isn't this already validated by the routes file?
There was a problem hiding this comment.
Yes, that seems to be the case.
| text: "Export Yearly Reports" | ||
| ) | ||
| %> | ||
| This will recalculate all the reports, and may take some time. |
There was a problem hiding this comment.
This might make sense to pair with #5495. Not sure if using this with actual data might time out right now.
There was a problem hiding this comment.
Yeah, that's reasonable. I saw that the PR was stale, so I just created a new PR based off it here: #5542. The new PR contains some changes based on the feedback. I think we should release that PR first to see that everything is working fine for distributions, and then I can update this PR once those changes are successfully integrated. How does that sound?
|
Thanks for reviewing, @dorner. Made the changes you requested 🙂. |
|
@angyb00 lint is failing - can you fix? Otherwise - @janeewheatley @ruestitch this is ready for QA. |
angyb00
force-pushed
the
4987-annual-survey-export
branch
from
43a30dd to
d6d8ef0
Compare
dorner
left a comment
dorner
left a comment
There was a problem hiding this comment.
Sorry @angyb00 I missed the fact that this was a single button on the UI. Why are we messing with route parameters, input arguments etc? The service class should handle the years in question without any inputs from the view or controller.
2 participants
Resolves #4987
Description
This builds off the work that was started in #5274 and #5199. This implementation fixes the issue of different years having different columns; all header columns are now included.
Type of change
How Has This Been Tested?
This has been tested through the UI and rspec.
Screenshots
Screen.Recording.2026-04-19.at.6.50.26.PM.mov